Conversation
|
🎉 Welcome to the Kubeflow Trainer! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
Pull Request Test Coverage Report for Build 21665617792Details
💛 - Coveralls |
|
/retest |
There was a problem hiding this comment.
Pull request overview
This PR adds Istio sidecar exclusion configuration and seccomp security profiles to improve compatibility with Pod Security Standards (PSS) restricted policies when using Istio service mesh. The changes ensure that webhook traffic on port 9443 bypasses the Istio sidecar and adds RuntimeDefault seccomp profiles for enhanced security.
Key Changes:
- Added Istio traffic exclusion annotation for inbound port 9443 to prevent sidecar interference with webhook communication
- Configured RuntimeDefault seccomp profiles at the pod security context level for compliance with restricted PSS
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| manifests/third-party/jobset/kustomization.yaml | Adds kustomize patch for jobset-controller-manager deployment with Istio annotation and seccomp profile |
| manifests/base/manager/manager.yaml | Updates trainer manager deployment with Istio exclusion annotation and seccomp security context |
| charts/kubeflow-trainer/templates/manager/deployment.yaml | Adds Istio traffic exclusion annotation to Helm chart template for trainer manager |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@andreyvelich i am waiting for this to merge kubeflow/manifests#3314 |
andreyvelich
left a comment
There was a problem hiding this comment.
/lgtm
@juliusvonkohout Can you rebase this PR to fix CI please ?
/cc @tenzen-y @astefanutti
Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
Add security context and annotations for Istio traffic management. Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
Added a patch to modify the jobset-controller-manager deployment annotations and security context. Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
Removed security context seccomp profile from deployment. Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
Removed security context seccompProfile configuration. Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
Removed security context seccompProfile from spec. Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
a2b6a53 to
6ec7112
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@andreyvelich rebase is done |
| annotations: | ||
| traffic.sidecar.istio.io/excludeInboundPorts: "9443" |
There was a problem hiding this comment.
Sorry for the delay @juliusvonkohout!
We discussed about it with @kannon92 on JobSet issue: kubernetes-sigs/jobset#1052 (comment)
Since other platforms might not use Istio (e.g. Cilium, Linkerd, etc.), I would suggest to add custom patch to the kubeflow-platform overlay to append this annotation alongside JobSet change:
trainer/manifests/third-party/jobset/kustomization.yaml
Lines 20 to 30 in 6ec7112
cc @kubeflow/kubeflow-trainer-team
There was a problem hiding this comment.
From Slack:
I don't suggest you to do that in kubeflow/manifests
Similar to JobSet, just update kubeflow-platform Kustomize overlay in kubeflow/trainer repo to add patch for kubeflow-trainer-controller-manager deployment:
trainer/manifests/third-party/jobset/kustomization.yaml
Lines 15 to 30 in 6ec7112
E.g. you can do that here: https://github.com/kubeflow/trainer/blob/6ec7112a3b397d0b160330f78f9b6bbcd4c59a0f/manifests/overlays/kubeflow-platform/kustomization.yaml
For the Helm Charts, simple use this Value to configure appropriate label when you do helm install
{{- include "trainer.manager.selectorLabels" . | nindent 8 }}
helm install kubeflow-trainer oci://ghcr.io/kubeflow/charts/kubeflow-trainer --version 2.1.0 --set trainer.manager.selectorLabels traffic.sidecar.istio.io/excludeInboundPorts=9443
Maybe a GSOC applicant can fork my branch and do the changes :-) and raise a PR against my the branch here. I will ask https://cloud-native.slack.com/archives/C0742LBR5BM/p1770634485333769
|
Might be continued in #3189 |
|
This PR is fixed by: #3189 |
|
@andreyvelich: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@andreyvelich may you also patch it in helm for the jobset ?